-
Notifications
You must be signed in to change notification settings - Fork 49
[Fix] Make androidx.metrics:metrics-performance:1.0.0-beta01 resolution more robust (RN <0.76) #937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…mance version selected by dd-sdk-android-rum
if (reactNativeMajorVersion == 0 && reactNativeMinorVersion < 76) { | ||
implementation("com.datadoghq:dd-sdk-android-rum:2.23.0") { | ||
exclude group: "androidx.metrics", module: "metrics-performance" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is still needed if resolutionStrategy
is used anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one makes sure that the right version gets used while compiling the SDK. Without it (and just with the resolution strategy that I just added) the build would fail because that resolution would arrive too late. I tested it in many ways and this was the only way to get it to work with the RN build pipeline.
The new addition is just there to make sure that if any other dependency included on the host app also introduces this dependency we resolve it to beta01, as the default resolution strategy is to choose the higher version, and we don't want that.
It's a bit convoluted, I know 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange, something is not right. You should have only implementation("com.datadoghq:dd-sdk-android-rum:2.23.0")
, without exclude
.
Try to put resolutionStrategy
call before dependencies are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also tried that, yes. It didn't work either. That way it would always end up using beta02 and ignoring the RN version.
That's why I had to use this approach of excluding android.metrics from com.datadoghq:dd-sdk-android-rum and then adding beta01 on my own. Obviously this is only for React Native versions under 0.76, for anything newer we simply do implementation "com.datadoghq:dd-sdk-android-rum:2.23.0"
on the line below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work, so it is very strange, probably something is not right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it another shot, see If I can figure out a way to make it work 🦾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbarrio Any updates on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, I'll let you know as soon as I have some.
In the meantime, I'm moving this back to draft to avoid confusion 😅
What does this PR do?
On #896, we added a clause on
build.gradle
to forceandroidx.metrics:metrics-performance:1.0.0-beta01
instead ofbeta02
when building with React Native versions under 0.76. However, we didn't acknowledge for this dependency to also be added by another module inside the host app where the SDK will be integrated, causing build issues once again.This PR adds an extra resolutionStrategy that enforces
beta01
to always be enforced if needed under React Native < 0.76.Motivation
To make the resolution of this dependency more robust and avoid breaking builds on older versions of React Native.
Additional notes
I also ran this change against the nightly tests to check builds across the board and it all works fine.
Review checklist (to be filled by reviewers)